refactor: 💡 use hds side nav w/ hds frame instead of rose #2688
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| {{/if}} | ||
| </Rose::Nav::Sidebar> | ||
| {{#if this.session.isAuthenticated}} | ||
| {{#in-element this.sideBarContainer}} |
There was a problem hiding this comment.
Why did we have to portal this in?
There was a problem hiding this comment.
thats a good point, i think the same thing can be accomplished using the side nav list directly.
There was a problem hiding this comment.
Edit: Just realized you are referring to the in-element helper usage. That is because we don't show the side nav on all routes but we do want the side nav to be inserted into the correct hds::frame component.
There was a problem hiding this comment.
My 2 cents tip: add a comment in the ui/admin/app/templates/application.hbs file, above the <Frame.Sidebar declaration, to explain what is going on. I wasn't sure how you were adding the content to the sidebar, until I read this comment above.
There was a problem hiding this comment.
I don't know if/how it would apply to your use case, but have a look at how Cloud UI has solved the problem of having some pages without a sidebar: https://github.com/hashicorp/cloud-ui/blob/main/addons/core/package/src/components/hcp-app-frame.gts#L148
(essentially, if you pass @hasSidebar={{a false value}} the sidebar is not rendered at all)
This may avoid doing double portalling.
lisbet-alvarez
force-pushed
the
refactor-rose-side-bar-to-use-hds
branch
from
e30d653 to
0583534
Compare
| @@ -53,3 +53,20 @@ form { | |||
| color: inherit; | |||
| } | |||
| } | |||
|
|
|||
| // TODO: This is a temporary fix that ensures changing the color theme won't alter the dropdown colors. | |||
| .hds-app-frame__sidebar .hds-side-nav { | |||
There was a problem hiding this comment.
Hds team is helping us find a cleaner solution for this style override. So this fix is just temporary.
| @@ -37,7 +37,7 @@ | |||
| <page.actions> | |||
| {{#if (or perms.canUpdate perms.canDelete)}} | |||
|
|
|||
| <Hds::Dropdown as |dd|> | |||
| <Hds::Dropdown data-test-manage-alias as |dd|> | |||
There was a problem hiding this comment.
Do we want to specify that this is the manage alias dropdown in the targets route?
There was a problem hiding this comment.
ah great point!
| const DROPDOWN_BUTTON_SELECTOR = | ||
| 'tbody tr td:last-child .hds-dropdown-toggle-icon'; |
There was a problem hiding this comment.
asking for learning: Did this need to be updated since we have more dropdown (specifically in the new side nav) elements on the screen? Either way, I think this is a good change.
There was a problem hiding this comment.
yup that is correct!
There was a problem hiding this comment.
@lisbet-alvarez I've left a few comments, I hope you don't mind
(as mentioned in one of the comments, I am currently working on auditing page/layout components across our products' codebases, hence some context about this PR)
| @@ -5,113 +5,23 @@ | |||
|
|
|||
| {{page-title (app-name)}} | |||
|
|
|||
| <Rose::Layout::Global as |layout|> | |||
| <Hds::AppFrame @hasHeader={{false}} as |Frame|> | |||
There was a problem hiding this comment.
Not sure how it works, but if you bring the same changes to the "desktop" app and remove/replace the <Rose::Layout::Global> instance from the playground.hbs file, at that point you can remove entirely the code (hbs, css, js, etc) of the Rose::Layout::Global component
(not sure if that would be outside of the scope of this PR)
There was a problem hiding this comment.
Similarly for the Rose::Layout::Sidebar which is only left in the playground and in this page
There was a problem hiding this comment.
(the reason I know it, is because in those days I am doing an audit of all the page/layout components used in our products codebases, and I noticed in Boundary these Two rose components (Rose::Layout::Global and Rose::Layout::Sidebar) and under each one of them, in my audit document, I've written: "Likely the combination of Global+Sidebar layout components can be replaced with a single Hds::AppFrame component" 🙂
| </Hds::AppFooter> | ||
| </sidebarLayout.footer> | ||
| </Rose::Layout::Sidebar> | ||
| <Rose::Layout::Centered> |
| {{/if}} | ||
| </Rose::Nav::Sidebar> | ||
| {{#if this.session.isAuthenticated}} | ||
| {{#in-element this.sideBarContainer}} |
There was a problem hiding this comment.
My 2 cents tip: add a comment in the ui/admin/app/templates/application.hbs file, above the <Frame.Sidebar declaration, to explain what is going on. I wasn't sure how you were adding the content to the sidebar, until I read this comment above.
| {{/if}} | ||
| </Rose::Nav::Sidebar> | ||
| {{#if this.session.isAuthenticated}} | ||
| {{#in-element this.sideBarContainer}} |
There was a problem hiding this comment.
I don't know if/how it would apply to your use case, but have a look at how Cloud UI has solved the problem of having some pages without a sidebar: https://github.com/hashicorp/cloud-ui/blob/main/addons/core/package/src/components/hcp-app-frame.gts#L148
(essentially, if you pass @hasSidebar={{a false value}} the sidebar is not rendered at all)
This may avoid doing double portalling.
Description
Refactor to use
Hds::SideNavandHds::AppFramecomponents.Note: There is an issue with the
Hds::SideNav::Listcomponent so we are using theHds::SideNav::Portalcomponent in the meantime.The side nav should not be visible on the
onboarding,change-password, andauthenticateroutes.Screenshots (if appropriate)
How to Test
a. user menu options such as sign-out, change-password, light mode, dark mode, etc.
b. dev tool toggle options such as feature editions and licensed features.
Checklist